Skip to content

revert js chart size#377

Merged
iceljc merged 1 commit into
SciSharp:mainfrom
iceljc:features/refine-chat-window
Aug 28, 2025
Merged

revert js chart size#377
iceljc merged 1 commit into
SciSharp:mainfrom
iceljc:features/refine-chat-window

Conversation

@iceljc
Copy link
Copy Markdown
Collaborator

@iceljc iceljc commented Aug 28, 2025 •
edited by qodo-code-review Bot
Loading

PR Type

Bug fix

Description

  • Restore chart container size constraints for JS interpreter

Diagram Walkthrough

flowchart LR
  A["Chart Container"] -- "restore size" --> B["Fixed Dimensions"]
  B --> C["min-width: 800px"]
  B --> D["max-height: 500px"]
Loading

File Walkthrough
Relevant files
Bug fix
rc-js-interpreter.svelte
Restore chart container size constraints                                 

src/routes/chat/[agentId]/[conversationId]/rich-content/rc-js-interpreter.svelte

  • Added inline style with minimum width and maximum height constraints
    to chart container div
 +1/-1     

@iceljc iceljc merged commit 20d5492 into SciSharp:main Aug 28, 2025
1 of 2 checks passed
@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Aug 28, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Responsiveness

Hard-coded inline styles (min-width: 800px; max-height: 500px) may break layout on small screens or within constrained containers; consider using responsive CSS classes or media queries instead of fixed inline sizes.

 
    <div id={`chart-${message?.message_id}`} style="min-width: 800px; max-height: 500px;"></div>
</div>
Inline Styles

Using inline styles reduces maintainability and theming; prefer a CSS class (e.g., utility classes or scoped styles) so constraints can be adjusted centrally and tested.

 
    <div id={`chart-${message?.message_id}`} style="min-width: 800px; max-height: 500px;"></div>
</div>

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Aug 28, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Make chart container responsive

Using a fixed min-width of 800px will cause horizontal overflow on smaller
screens, breaking layout and usability. Make the container responsive by capping
it at 800px while allowing it to shrink to the viewport width. This keeps the
chart readable without forcing scroll on mobile.

src/routes/chat/[agentId]/[conversationId]/rich-content/rc-js-interpreter.svelte [103]

-<div id={`chart-${message?.message_id}`} style="min-width: 800px; max-height: 500px;"></div>
+<div id={`chart-${message?.message_id}`} style="width: min(100%, 800px); max-height: 500px;"></div>
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that a fixed min-width of 800px will cause horizontal overflow on smaller screens and proposes a valid responsive solution using width: min(100%, 800px).

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 1/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@iceljc